Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tree] test for limits in buildindex #1090

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ferdymercury
Copy link
Contributor

@ferdymercury ferdymercury commented Mar 18, 2024

@phsft-bot

This comment was marked as outdated.

1 similar comment
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

1 similar comment
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some questions regarding the implementation of the test

root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
root/tree/index/runindex64.C Outdated Show resolved Hide resolved
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@pcanal
Copy link
Member

pcanal commented Apr 9, 2024

If finding an entry in the index is platform-dependent, isn't that a problem?

Indeed, the entry should be the same or at least the one intended otherwise there is a problem.

Is there a way in the run.C mechanism to express platform dependency with different logs?

There is several ways. It easily supports 32bits vs 64bits and interpreted vs compiled but not really this vs that platform. There is way to filter/modify the log before comparison.

However, the usual way to deal with the platform difference is to test the values directly within the code rather than indirectly via printing (the later works well when comparing a large number of values that are not platform dependent).

-4: Run 5, Event 1152921504606846975 found at entry number: 4
+4: Run 5, Event 281474976710655 found at entry number: 4

Here the difference is indeed spurrious as the intended entry number is found. We could either not print the Run Number/Event Number (as the existing code did) or only print them if the entry number is not 4.

-5: Run 5, Event 18446744073709551600 found at entry number: -1
+5: Run 5, Event 4503599627370480 found at entry number: 5

The difference here seems to indicate an actual error (either in the test or in the code). (The reference file in the master seems to indicates that this used to work)

@ferdymercury
Copy link
Contributor Author

(The reference file in the master seems to indicates that this used to work)

The reference file was casting to 32bit a number that was longer than 32bits so it was just working by chance, I believe.

@ferdymercury
Copy link
Contributor Author

ferdymercury commented Apr 10, 2024

We could either not print the Run Number/Event Number

Yep I can change that. But first I wanted to understand the issue with Event5 not being found.

@phsft-bot

This comment was marked as outdated.

1 similar comment
@phsft-bot

This comment was marked as outdated.

@ferdymercury ferdymercury dismissed stale reviews from pcanal and vepadulano April 17, 2024 09:39

Changes done

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

1 similar comment
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

as it gives problems with the Error output of GetIndex when comparing with the .ref

clarify wording

and restore platform-dependency

no need for minor/major limits
apply pcanals suggestion

only print event entries if error

align indentation of values for easier reading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants